-
Notifications
You must be signed in to change notification settings - Fork 14.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: SIP-34 explore save modal #10355
Conversation
9cd6cd6
to
3de1179
Compare
@mistercrunch see north star design for add chart to new dashboard For the After image: The text field under radio buttons would benefit by adding a title so that its clear what the field is for. See SIP-34 screenshot you posted. Also I think "Save Chart" is more direct than "Save a Chart". "Save a Chart" implies unnecessary ambiguity in the statement. |
3de1179
to
659c596
Compare
@Steejay , I decided to use the "CreatableSelect" component feature from react-select to avoid having to build the select box. I also improved the e2e cypress tests quite a bit, fixed and re-enabled tests that had been disabled. |
Codecov Report
@@ Coverage Diff @@
## master #10355 +/- ##
==========================================
- Coverage 69.70% 65.42% -4.29%
==========================================
Files 196 603 +407
Lines 18950 32393 +13443
Branches 0 3287 +3287
==========================================
+ Hits 13210 21194 +7984
- Misses 5740 11015 +5275
- Partials 0 184 +184
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
d02f208
to
196a73b
Compare
The more I look at this the more I'm agonizing over the Could it just be:
^ Just displaying an overwrite warning, rather than forcing a choice? |
I don't think this works as two charts can have the same title. Sometimes users just want to save a new copy of a chart and play with it. I also feel |
@ktmud I don't have data to back this up, but the "save AND create a new dashboard" is not super common, it's still very much possible and discoverable IMHO. I think the previous modal looks awful and is super confusing. I'd hate to shelf this because it's not 100% SIP-34. This PR also re-enables/improves many unit tests and removes a lot of unneeded complexity. @ktmud if the concern is the "create dashboard" is not discoverable enough, I can think of ways to improve that, like bolding the words "Create" and "Select" in the placeholder of the dashboard dropdown |
I prefer option 1. All caps is a little too aggressive/dominating. Additionally, the header should read "Save Chart" not "Save A Chart". Can we add the title "Chart Name*" above the text field as well? see comments in #10355 (comment) |
196a73b
to
8ea461a
Compare
11fa167
to
32b0f52
Compare
@ktmud I'd love your blessing on this, but I think this is solid mergeable progress and a good checkpoint in the right direction |
This design is definitely much cleaner than the existing layout. I'm OK with merging as is but would love to see the new iteration soon. A placeholder text still doesn't look as discoverable as the inline input in SIP-34 or the radio buttons in old design. Another problem with current |
Impacts #8976 |
* feat: SIP-34 explore save modal * using a const for the session storage key * backend changes * minor tweaks * more tweaks * radio cosmetics * styles * fix tests * CreatableSelect\! * Fix cypress & lint * fix unit * lint
Getting the explore save modal closer to SIP-34
RFC: SIP-34 doesn't really offer the "Create a new dashboard and add this chart into it" option and I wanted to preserve that. I did it with a checkbox + leaving the select empty, but unclear what the best way would be.
After (updated to latest commit)
Before
SIP-34